New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the Audit Trail API #407
Conversation
7d7df59
to
8385afb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see all this implemented within a new type of Client, not as a part of existing type Client
. Naming is tricky but perhaps something like JsonClient or AuditTrailsClient.
Rationale:
- The endpoint is incompatible with the existing auth and type headers
- The endpoint requires different token config (always an org token, which is rare)
What if the API was more like
auditConfig := DefaultConfig()
auditConfig.Token = orgToken
auditClient, err := NewAuditTrailClient(&auditConfig)
auditTrails, err := auditClient.AuditTrails.List(ctx)
one thing to look at to help with naming is if there is ANY OTHER public API that doesn't use json:api.
It just doesn't feel right to drop to retryablehttp, reusing client, within this method.
8385afb
to
8a42e3d
Compare
audit_trail_integration_test.go
Outdated
client := testClient(t) | ||
ctx := context.Background() | ||
|
||
orgToken := os.Getenv("TFE_ORGANIZATION_TOKEN") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you decide that creating an org token would not work? I'm sorry I forgot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we currently have no way in go-tfe to upgrade an organization to the business tier. The integration tests in atlas do in fact upgrade the entitlement set for an org but it's an internal endpoint as I understand it.
I discussed with @taiidani and @annawinkler on potentially configuring tflocal to default any organization that's created to the business plan so we can run the entire test suite. This is non trivial and requires further internal discussion. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non go-tfe endpoint that the Atlas integration tests use: https://github.com/hashicorp/atlas/blob/32947de5536a9df4fb79ce18868b0b4ad29bfa07/integration-tests-api/internal/testenv/helpers.go#L775-L838
e40193f
to
9958a1d
Compare
Is this waiting on #415 ? |
440c399
to
9c36e74
Compare
// AuditTrailListOptions represents the options for listing audit trails. | ||
type AuditTrailListOptions struct { | ||
// Optional: Returns only audit trails created after this date | ||
Since time.Time `url:"since,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, this looks like it should be a pointer since it is an emptyable struct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The zero value for time.Time{}
is 0001-01-01 00:00:00 +0000 UTC
so it wouldn't be an issue. The only reason I wouldn't make it a pointer is time.Time
isn't really ever used as a pointer.
CHANGELOG.md
Outdated
# v1.3.0 | ||
|
||
## Enhancements | ||
* Add support for Audit Trail API by @sebasslash [#407](https://github.com/hashicorp/go-tfe/pull/407) | ||
|
||
# v1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.2.0 is still called Unreleased in main 😱
ab0323e
to
5b904cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An orgToken parameter still doesn't sit right with me on the interface API. It's confusing because you already supplied an auth token when you created the client, and it feels like configuration as opposed to a list option.
Can we re-open the argument for having a second 'ComplianceClient' or 'PlatformClient' that consumes json instead of jsonapi and is configured with a separate token?
@@ -66,7 +66,7 @@ This API client covers most of the existing Terraform Cloud API calls and is upd | |||
- [x] Agent Pools | |||
- [x] Agent Tokens | |||
- [x] Applies | |||
- [ ] Audit Trails | |||
- [x] Audit Trails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found that a user token will authenticate, returning an empty List and 200 response.
|
||
headers := make(http.Header) | ||
headers.Set("Authorization", "Bearer "+s.client.token) | ||
headers.Set("Content-Type", "application/json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should set the user-agent to go-tfe
, similar to tfe.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebasslash Is this something that still needs to be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in L89: headers.Set("User-Agent", _userAgent)
where _userAgent
is go-tfe
I think we should set the user agent on this before merging
1e52b52
to
efe4879
Compare
6b37dbe
to
3e459a1
Compare
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
Adds support for the Audit Trail API. One thing to note is that this particular API requires an organization token and does not follow the JSON+API spec as most of our endpoints do. Thusly the implementation of the
List()
method will create a new retryable http client and send the request using that client -- I figured this would be better than altering the existing client that is shared across our interfaces.Testing plan
To run the integration test:
go test -v ./... -run TestAuditTrailsList -tags=integration
Output from tests